Walkthrough

This pull request adds support for optional compression levels in the Avro C++ DataFile implementation. The changes extend the DataFileWriter API to accept an optional compression level parameter, update DEFLATE and ZSTD compression routines to utilize this parameter, and introduce codec validation and availability utilities. The Codec enum is modified to unconditionally define SNAPPY_CODEC and ZSTD_CODEC. ZstdCompressWrapper's compress method signature is updated to accept an optional compression level. Corresponding public API declarations and constructor signatures are updated across DataFile.hh, and comprehensive test functions are added to validate codec behavior and compression round-trips.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

PR Review: AVRO-4222 - Support writer to specify compression level

Summary

This PR adds support for specifying compression levels when writing Avro data files in C++. The implementation looks solid overall with good test coverage and proper validation.

✅ Strengths

1. Comprehensive Test Coverage - Excellent property-based testing approach:

   • Random compression level validation (100 iterations)
   • Random round-trip testing with various data sizes
   • Tests for codec availability and enum values
   • Both DEFLATE and ZSTD codecs covered

2. Good API Design:

   • Uses std::optional<int> for compression level (modern C++ best practice)
   • Defaults to codec-specific defaults when not specified
   • Clear separation of concerns with validateCodec() and isCodecAvailable()

3. Proper Validation:

   • Validates compression levels at construction time
   • Clear error messages indicating valid ranges
   • Codec availability checks before use

4. Backward Compatibility:

   • Optional parameter defaults to std::nullopt
   • Existing code continues to work without changes

🔍 Issues & Suggestions

Critical Issues

1. Duplicate codec metadata setting (lang/c++/impl/DataFile.cc:175-177)

   validateCodec(codec_, compressionLevel_);
   setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);  // Line 176 - Always sets null
   if (codec_ == NULL_CODEC) {
       setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);  // Line 178 - Duplicate!

   The metadata is unconditionally set to AVRO_NULL_CODEC at line 176, then immediately overwritten. Remove line 176.

2. Missing header include (lang/c++/impl/DataFile.cc)
   The code uses std::optional but doesn't explicitly include <optional>. While it may work through transitive includes, it should be explicitly included for portability:

   #include <optional>

Code Quality

3. Inconsistent codec validation logic (lang/c++/impl/DataFile.cc:67-109)

   • The validateCodec() function silently ignores compression levels for NULL_CODEC and SNAPPY_CODEC
   • Consider logging or warning users that the level is being ignored
   • Alternative: Throw an exception if level is specified for codecs that don't support it (stricter validation)

4. Codec name formatting inconsistency (lang/c++/impl/DataFile.cc:67-73)
   The codecToString() helper returns "zstandard" but metadata uses AVRO_ZSTD_CODEC (presumably "zstd"). Ensure consistency with Avro spec naming conventions.

5. Missing const correctness (lang/c++/impl/DataFile.cc:67-73)
   The codecToString() function could be constexpr for compile-time evaluation:

   constexpr const char* codecToString(Codec codec) {
       // ... implementation
   }

Test Improvements

6. Non-deterministic test seeding (lang/c++/test/DataFileTests.cc:1289, 1330)

   boost::mt19937 rng(static_cast<uint32_t>(time(nullptr)));

   Using time(nullptr) makes tests non-reproducible. Consider using a fixed seed or allowing seed to be specified via environment variable for debugging:

   const char* seed_env = std::getenv("AVRO_TEST_SEED");
   uint32_t seed = seed_env ? std::atoi(seed_env) : static_cast<uint32_t>(time(nullptr));
   boost::mt19937 rng(seed);
   std::cout << "Using seed: " << seed << std::endl;

7. Edge case testing gaps:

   • No test for compressionLevel = 0 for DEFLATE (valid but edge case)
   • No test for large file sizes to verify compression actually works under memory pressure
   • No test verifying that different compression levels produce different compressed sizes

Documentation

8. Missing documentation for new parameter:
   • Add Doxygen comments for compressionLevel parameter in DataFileWriterBase constructor
   • Document valid ranges in header file
   • Example usage would be helpful

🔒 Security Considerations

No security concerns identified. The validation properly prevents:

• Integer overflow (bounded ranges)
• Invalid codec usage (availability checks)
• Buffer issues are handled by underlying zlib/zstd libraries

⚡ Performance Considerations

1. Compression level validation overhead: Validation happens at construction time, so no runtime overhead during writes - good design.

2. Memory allocation in ZstdCompressWrapper: The code uses ZSTD_compressBound() to pre-allocate buffers, which is correct. No concerns here.

3. Default compression levels: Using defaults (Z_DEFAULT_COMPRESSION, ZSTD_CLEVEL_DEFAULT) when unspecified is appropriate for backward compatibility.

📋 Recommendations

Must fix before merge:

1. Remove duplicate setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC) at line 176

Should fix:
2. Add explicit #include <optional> in DataFile.cc
3. Improve test reproducibility with configurable seeds
4. Add documentation for the new parameter

Nice to have:
5. Consider stricter validation for unsupported codec/level combinations
6. Add edge case tests (level 0, compression ratio verification)
7. Make codecToString() constexpr

📊 Overall Assessment

Quality: 8/10 - Well-implemented feature with excellent test coverage, but needs the critical duplicate metadata bug fixed.

The PR demonstrates good understanding of:

• Modern C++ practices (std::optional, move semantics)
• Property-based testing methodology
• API backward compatibility
• Error handling and validation

Great work overall! Please address the critical metadata issue and consider the other suggestions.

176-188: Redundant metadata assignment.

Line 177 unconditionally sets AVRO_CODEC_KEY to AVRO_NULL_CODEC, but lines 178-188 immediately overwrite it based on the actual codec. The initial assignment on line 177 is unnecessary.

value:good-to-have; category:bug; feedback:The CodeRabbit AI reviewer is correct! There is no need to check whether the codec is AVRO_NULL_CODEC because it is set as a default anyway. Prevents re-setting the AVRO_NULL_CODEC a second time.

1. Duplicate codec metadata setting (lang/c++/impl/DataFile.cc:175-177)

   validateCodec(codec_, compressionLevel_);
   setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);  // Line 176 - Always sets null
   if (codec_ == NULL_CODEC) {
       setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);  // Line 178 - Duplicate!

   The metadata is unconditionally set to AVRO_NULL_CODEC at line 176, then immediately overwritten. Remove line 176.

value:good-to-have; category:bug; feedback:The Claude AI reviewer is correct! There is no need to check whether the codec is AVRO_NULL_CODEC because it is set as a default anyway. Prevents re-setting the AVRO_NULL_CODEC a second time.

2. Missing header include (lang/c++/impl/DataFile.cc)
The code uses std::optional but doesn't explicitly include <optional>. While it may work through transitive includes, it should be explicitly included for portability:
c++ #include <optional>

value:useful; category:bug; feedback:The Claude AI reviewer is correct! This include is missed in two .cpp files. The build passes because the import comes as a transitive from another import. Prevents a build break if the transitive dependency stops using std::optional and removes its include for it.